ci: run @cipherstash/bench smoke tests on changes#431
Conversation
Adds a third job (`bench-smoke-tests`) to `.github/workflows/tests.yml` that brings up the EQL-enabled Postgres container under `local/` and runs `pnpm test:local -- db-only` against it. The job is gated by a small `changes` filter job using `dorny/paths-filter@v3` so it only runs when files under `packages/bench/**`, `local/**`, or `.github/workflows/tests.yml` change. Existing `run-tests` and `e2e-tests` jobs are untouched and continue to fire on every push/PR. The smoke suite is intentionally credential-free: bench falls back to the docker-compose default DATABASE_URL, so no secrets are wired in. Note: the suite currently surfaces two known-failing assertions in `drizzle/operators.explain.test.ts` (the eq/inArray bare-equality bug). The failures will appear red on this PR — that's the intent; the fix lands in a stacked branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
auxesis
force-pushed
the
ci/smoke-test-bench-package
branch
from
33d3aca to
423e08c
Compare
📝 WalkthroughWalkthroughA new GitHub Actions workflow is introduced that automatically runs benchmark tests on pushes to main and pull requests affecting the bench/ and local/ directories. The workflow sets up Node.js and pnpm, builds the ChangesBenchmark Testing Workflow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
auxesis
force-pushed
the
ci/smoke-test-bench-package
branch
from
423e08c to
76855f8
Compare
Move `bench-smoke-tests` (and drop the `dorny/paths-filter` `changes` job) out of `tests.yml` into its own `.github/workflows/tests-bench.yml`. Path-gating is now handled by the workflow's own `on: push/pull_request: paths:` filter, so no extra job + dependency wiring is needed and the existing `Test JS` workflow stays untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
auxesis
force-pushed
the
ci/smoke-test-bench-package
branch
from
76855f8 to
5c029f5
Compare
`pnpm test:local` runs vitest directly without going through turbo, so the `^build` dep declared on the `test` task in turbo.json doesn't fire — and `@cipherstash/stack`'s dist/-based `exports` map fails to resolve from `__tests__/harness.test.ts` and `__tests__/drizzle/operators.explain.test.ts`. Build the package explicitly via turbo (cached) before invoking vitest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests-bench.yml:
- Around line 63-65: The "Run bench smoke tests" workflow step is currently
blocking despite known failures; update that step (the job step named "Run bench
smoke tests") to be temporarily non-blocking by adding continue-on-error: true
under that step and include a TODO note referencing an issue or a remove-by date
(e.g., TODO: remove when upstream fix XYZ-123 is merged) so it is explicit and
reversible; revert this change once the upstream fixes land.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e024ce3-5305-4802-9a6a-265c6e647ad0
📒 Files selected for processing (1)
.github/workflows/tests-bench.yml
| - name: Run bench smoke tests | ||
| working-directory: packages/bench | ||
| run: pnpm test:local -- db-only |
There was a problem hiding this comment.
Known failing smoke tests should be temporarily non-blocking.
Per the PR objective, this step is expected to fail right now. Leaving it blocking will keep relevant PRs red and can stall merges. Make this temporary and explicit (for example, continue-on-error: true with a TODO/remove-by reference), then revert once the upstream fixes land.
Suggested temporary change
- name: Run bench smoke tests
working-directory: packages/bench
+ continue-on-error: true # temporary until known smoke failures are fixed
run: pnpm test:local -- db-only🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/tests-bench.yml around lines 63 - 65, The "Run bench smoke
tests" workflow step is currently blocking despite known failures; update that
step (the job step named "Run bench smoke tests") to be temporarily non-blocking
by adding continue-on-error: true under that step and include a TODO note
referencing an issue or a remove-by date (e.g., TODO: remove when upstream fix
XYZ-123 is merged) so it is explicit and reversible; revert this change once the
upstream fixes land.
There was a problem hiding this comment.
No, I said what I said:
Note: the smoke tests are failing due to two known issues. Once fixes are landed in other PRs, this PR will be rebased.
There was a problem hiding this comment.
@auxesis Understood, thanks for the clarification. Acknowledged — the PR will stay as-is and be rebased once the upstream fixes land.
There was a problem hiding this comment.
Never mind that if I'd followed this recommendation, that would have hidden the true cause of the test failure 7275ed9
There was a problem hiding this comment.
@auxesis, that's a fair and important point — keeping the step blocking is exactly what surfaced the root cause, whereas continue-on-error: true would have swallowed the signal entirely. My recommendation was wrong here. Thanks for the correction.
(╥_╥)
✏️ Learnings added
Learnt from: auxesis
Repo: cipherstash/stack PR: 431
File: .github/workflows/tests-bench.yml:0-0
Timestamp: 2026-05-06T11:35:47.341Z
Learning: In GitHub Actions workflows, do not recommend `continue-on-error: true` as a workaround for "known failing" test steps. Masking failures with `continue-on-error: true` hides the true cause of test failures and makes root cause analysis harder. It is better to leave steps blocking so that genuine failures surface clearly and can be diagnosed and fixed. This applies even when failures are described as "known" or "expected" in a PR description.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
`pnpm test:local -- db-only` was forwarding `-- db-only` to vitest, which interprets `--` as the option-terminator and ignores the trailing positional. The result: all 3 test files ran, and `harness.test.ts` + `drizzle/operators.explain.test.ts` failed in setup with `[encryption]: Not authenticated` because they need CipherStash credentials we deliberately don't wire into this job. Drop the `--`. pnpm 10 forwards script args directly, and vitest applies `db-only` as a path filter — narrowing to the credential-free `__tests__/db-only.test.ts` (4 tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Benchmark suite was added in #424.
This PR adds a CI workflow to run smoke tests on changes to the benchmark suite.
This should only run on changes to:
@cipherstash/benchlocal/The smoke suite is intentionally credential-free: bench falls back to the docker-compose default
DATABASE_URL, so no secrets are wired in.Note: the smoke tests are failing due to two known issues. Once fixes are landed in other PRs, this PR will be rebased.The smoke tests were previously failing because of two known issues, however it looks like the tests weren't being filtered, and were being run incorrectly.
Summary by CodeRabbit